Rework long.ToMetric for correct interpretation of decimals parameter#1700
Rework long.ToMetric for correct interpretation of decimals parameter#1700logiclrd wants to merge 7 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dafb015aea
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if ((divisor > 1) && (fractionalPart >= divisor / 2)) | ||
| number += Math.Sign(number); |
There was a problem hiding this comment.
The new decimals == 0 path rounds whenever fractionalPart >= divisor / 2, which always rounds halves away from zero. That makes midpoint cases inconsistent with the Math.Round behavior used by the double/int overloads (and by the new test expectation generator): for example, 2500L.ToMetric(decimals: 0) will become 3k instead of the ToEven result 2k (similarly -2500L becomes -3k). Since this change is specifically about aligning long rounding semantics, these .5 cases are now inaccurate.
Useful? React with 👍 / 👎.
|
@codex Review please. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc29f960ae
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Oops. Made a last-minute change before the preceding commit, didn't re-test. That'll teach me. |
|
@codex Review please. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31e79ad7be
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (fractionalPart > roundingPoint) | ||
| number += Math.Sign(number); | ||
| else if (fractionalPart == roundingPoint) | ||
| { | ||
| // Use banker's rounding for consistency with Math.Round used elsewhere on floats. | ||
| number += Math.Sign(number % 2); | ||
| } |
There was a problem hiding this comment.
After the decimals == 0 rounding step, number can become 1000 (or -1000) but the code immediately returns with the current unitText, producing outputs like 1000k for inputs such as 999500. This is inconsistent with the double/int path (which re-normalizes after rounding) and yields an incorrect metric unit at scale boundaries; a post-rounding normalization check is needed before formatting.
Useful? React with 👍 / 👎.
|
@codex Review please. |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
… be much more comprehensive by using a TheoryData generator, reworking it to follow the correct interpretation of the decimals parameter. Reworked ToMetric(long, ..) and BuildRepresentation(long, ..) in MetricNumeralExtensions.cs to treat the decimals parameter as a precision rather than a requested fixed number of decimals, making all the new tests pass. # Conflicts: # src/Humanizer/MetricNumeralExtensions.cs # tests/Humanizer.Tests/MetricNumeralTests.cs
…ts.cs to exercise cases where the rounding direction matters, with values that match expected behaviour for int.ToMetric and double.ToMetric (banker's rounding). Updated BuildMetricRepresentation in MetricNumeralExtensions.cs to use banker's rounding in the case where the fractional part rounds away entirely into the integer part of the number.
…ts.cs to exercise scale changes necessitated by rounding (999.9 => 1000). Updated BuildMetricRepresentation in MetricNumeralExtensions.cs to detect & handle when rounding has bumped the number from +/-999 to +/-1000.
…ralExtensions.cs that discovers scale to not force decimals onto numbers that don't require scaling.
ebcbdb9 to
4ffedba
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughSummary by CodeRabbit
Walkthroughlong.ToMetric now treats decimals as precision like int.ToMetric: zero and scale-0 values format as raw input, BuildMetricRepresentation was rewritten to compute fractional digits, perform carry-propagation rounding (banker’s tie handling), trim trailing zeros, and tests were converted to dynamically generated cases. ChangesDecimals parameter and rounding behavior
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Rebased. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Humanizer/MetricNumeralExtensions.cs (1)
205-212: ⚡ Quick winUpdate the
decimalsXML docs for thelongoverload.This overload’s behavior changed, but the parameter docs still describe
decimalsas a fixed decimal count. Please document that it now matches theint/doubleoverload semantics and trims unnecessary trailing zeros.As per coding guidelines, "Add XML documentation for new or modified public APIs in C#."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Humanizer/MetricNumeralExtensions.cs` around lines 205 - 212, Update the XML docs on the long overload ToMetric(this long input, MetricNumeralFormats? formats = null, int? decimals = null) to describe the new semantics of the decimals parameter: note that it now follows the same behavior as the int/double overloads (acts as a maximum number of decimal places rather than a fixed count) and that the resulting representation will trim unnecessary trailing zeros; edit the <param name="decimals"> tag in MetricNumeralExtensions.ToMetric to clearly state these points and mention default behavior when null.tests/Humanizer.Tests/MetricNumeralTests.cs (1)
84-213: 🏗️ Heavy liftKeep the generated expectations independent from the implementation.
Most rows here rebuild the same scale/round/trim behavior that
long.ToMetricnow uses. That weakens the test oracle: if the production bug is copied into this generator, the theory still passes. Prefer deriving these cases from an independent reference where possible, such as parity checks againstint.ToMetric/double.ToMetricon overlapping ranges, and keep the carry/scale transitions as explicit rows.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Humanizer/MetricNumeralExtensions.cs`:
- Around line 390-405: The current rounding in the BuildMetricRepresentation
path for ToMetric(long ...) uses a simple half-up check
(fractionalPartCharacters[decimals.Value] >= '5'); change it to banker's
(ToEven) rounding for decimals > 0 by detecting the exact-half case: when the
digit at fractionalPartCharacters[decimals.Value] == '5' and all subsequent
fractionalPartCharacters after that are '0', then only round up if the last
retained digit (fractionalPartCharacters[decimals.Value - 1]) is odd; otherwise
leave it (no round); for non-exact-half cases (digit > '5' or trailing
non-zero), continue to round up as currently implemented while preserving the
existing carry loop and break logic.
---
Nitpick comments:
In `@src/Humanizer/MetricNumeralExtensions.cs`:
- Around line 205-212: Update the XML docs on the long overload ToMetric(this
long input, MetricNumeralFormats? formats = null, int? decimals = null) to
describe the new semantics of the decimals parameter: note that it now follows
the same behavior as the int/double overloads (acts as a maximum number of
decimal places rather than a fixed count) and that the resulting representation
will trim unnecessary trailing zeros; edit the <param name="decimals"> tag in
MetricNumeralExtensions.ToMetric to clearly state these points and mention
default behavior when null.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fe233a92-5101-477b-b6ae-90bb50165428
📒 Files selected for processing (2)
src/Humanizer/MetricNumeralExtensions.cstests/Humanizer.Tests/MetricNumeralTests.cs
…cs to exercise exact midpoint rounding cases. Implemented Banker's rounding for the handling of the fractional part in the long.BuildMetricRepresentation extension method in MetricNumeralExtensions.cs.
…s in CoverageGapTests.cs to track the correction in the interpretation of the decimals parameter to long.ToMetric.
|
@clairernovotny Is there any chance this PR might be considered? 🙂 |
1 participant
This PR:
Replaces the test suite for the long.ToMetric extension method with a much more comprehensive set using a
TheoryDatagenerator, and uses an interpretation of thedecimalsparameter in line withint.ToMetricanddouble.ToMetricin the process.Makes all the tests pass by reworking the implementation of the
ToMetric(long, ..)extension method and its underlying helperBuildRepresentation(long, ..)correspondingly.This amends the implementation merged in #1596 to correct this misinterpretation. Thanks to @petervanleeuwen for noticing it 🙂
Here is a checklist you should tick through before submitting a pull request:
ToMetric(this long, ..), there is a one-line block with braces. The reason is consistency with the existing code inToMetric(this int, ..)andToMetric(this double, ..). If you prefer, I can add a commit that updates all of them to follow this style point.mainbranch (more info below)fixes #<the issue number>build.cmdorbuild.ps1and ensure there are no test failuresbuild.cmdorbuild.ps1in the source tree, but the solution builds and all tests discovered by the Test Explorer pass.Fixes: #1699